Skip to content

Conversation

@ar1ja
Copy link
Contributor

@ar1ja ar1ja commented May 16, 2022

No description provided.

@DannyBen
Copy link
Member

Thanks. Why is this better?

@ar1ja
Copy link
Contributor Author

ar1ja commented May 16, 2022

Thanks. Why is this better?

Hey :)

  • The shbang is useless and the top helps with docs
  • It keeps some bash features for completion, like it gives some autocompletion in some way
  • Ex notation is recognised by a lot more editors
  • Sometimes completion might fail and you might get an error

@DannyBen
Copy link
Member

DannyBen commented May 16, 2022

Ok. What do we gain from this:

-o bashdefault -o default

and more importantly, could this be NOT desired in some cases?
I do not see it in any other completion script (while I do see the other proposed changes).

@ar1ja
Copy link
Contributor Author

ar1ja commented May 16, 2022

Ok. What do we gain from this:

-o bashdefault -o default

and more importantly, could this be NOT desired in some cases? I do not see it in any other completion script (while I do see the other proposed changes).

The always complete thing

@DannyBen
Copy link
Member

The always complete thing

I don't understand what that means. What behavior does it add exactly.

Also this line:

 _init_completion -s || return

breaks the tests. Specifically this test:

https://github.com/DannyBen/completely/blob/8819f5610618090df754175668577b1971d1b37c/spec/completely/integration_spec.rb#L12-L16

The result is an empty array. Also tried defining the locals before the _init_completion, but same.

@DannyBen
Copy link
Member

I think I found the problem. Need to source source /usr/share/bash-completion/bash_completion in the test script.

I will probably create a new PR with the fixed tests to supersede this one, but unless there is a good reason to keep thises -o bashdefault -o default, I might remove them.

@DannyBen DannyBen mentioned this pull request May 16, 2022
@DannyBen
Copy link
Member

I am closing this in favor of #13 - let's continue discussion there.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants